Skip to content

fix(runtime): cancel standalone runs on process signals - #80

Merged
carldebilly merged 16 commits into
yllibed:mainfrom
autocarl:agent/issue-79-signal-handling
Sep 10, 2026
Merged

fix(runtime): cancel standalone runs on process signals#80
carldebilly merged 16 commits into
yllibed:mainfrom
autocarl:agent/issue-79-signal-handling

Conversation

@autocarl

@autocarl autocarl commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • bridge the first Ctrl+C console event—or Ctrl+Break on Windows—received by process-owning standalone ReplApp.Run(...) / RunAsync(...) calls into cooperative handler cancellation
  • also bridge the first SIGTERM on supported Unix platforms
  • coordinate signal ownership and first/second-signal escalation through a single process-wide epoch shared by overlapping runs
  • preserve the interactive command-cancellation policy through the same console-key broker, including atomic acquisition and teardown hand-offs
  • preserve caller-owned behavior for an unprofiled ReplApp.Create(), embedded profiles, and externally managed host/provider overloads
  • let cancellation callbacks drain before the signal epoch resets and resolve the final exit code only after signal ownership is released
  • diagnose unsupported bridges, registration failures, ignored external-host Automatic requests, and consumer cancellation-callback failures
  • document exit-code conventions, token lifetime, host ownership, diagnostics, escalation, and platform limits

Fixes #79.

Ownership model

ReplRunOptions.ProcessSignalHandling is nullable. null inherits the active app/profile default:

  • UseCliProfile() / UseDefaultInteractive(): Automatic
  • unprofiled ReplApp.Create(): None
  • UseEmbeddedConsoleProfile(): None

Supplying unrelated options therefore no longer makes an embedded console silently install standalone process handlers. An embedded or unprofiled caller can still opt in explicitly for one run.

Automatic applies to standalone overloads that use internally configured services. External IServiceProvider, IHost, and IReplHost overloads retain ownership of process signals. Their one-shot handlers receive the caller token unchanged. An explicit Automatic request on those overloads is diagnosed and ignored. If one of those runs enters Repl's interactive loop, each command receives a command-scoped token linked to the caller token so the loop can retain its separate Ctrl+C policy.

The process registrations are installed lazily once and remain inert without an active automatic run. This avoids runtime callback-snapshot races during registration teardown. A first signal atomically claims an epoch, cancels every active scope, and immediately cancels late joiners. A second signal falls through to the operating-system default. The epoch resets only after its final scope and all signal-triggered cancellation callbacks have drained.

Cancellation-callback draining is intentionally unbounded. Resetting an epoch while a callback remains active could cause a later signal to be suppressed as a new first signal. If draining never completes, every later supported signal falls through to operating-system termination.

Android, browser, iOS (including Mac Catalyst), and tvOS diagnose the unavailable bridge and leave cancellation to their platform host. .NET classifies Mac Catalyst in its iOS-like mobile family and compiles the platform-not-supported POSIX signal registration there.

Exit behavior

  • SIGINT / Ctrl+C returns 130 (128 + 2)
  • Ctrl+Break returns 130 as a Repl compatibility policy on Windows
  • SIGTERM returns 143 (128 + 15)
  • an explicit non-zero handler result takes precedence over the signal code
  • a second signal is not suppressed, so cleanup is not guaranteed to finish
  • no automatic grace-period timeout is imposed
  • Unix SIGQUIT is left unclaimed; .NET surfaces it as ControlBreak, but Repl does not reinterpret it as SIGINT

The 128 + signal number calculation is documented as a widely adopted Unix/Bash convention, not a universal POSIX, .NET, or Windows guarantee.

Regression coverage

Real child-process tests cover:

  • synchronous and asynchronous CLI-profile runs
  • SIGINT cooperative cleanup and exit 130
  • SIGTERM cooperative cleanup and exit 143
  • ProcessSignalHandlingMode.None
  • explicit non-zero handler exit-code precedence after cancellation
  • second-SIGTERM forced termination during deliberately blocked cleanup
  • captured stdout/stderr diagnostics and bounded child cleanup

Deterministic unit tests cover:

  • all already-active scopes receiving the first signal
  • late scopes inheriting the draining epoch
  • exactly one winner when two signals race
  • interactive ownership replacement in both acquisition/disposal orders
  • the pre-unregister disposal window remaining signal-owned
  • epoch retention while cancellation callbacks are blocked
  • reentrant scope creation from a cancellation callback without deadlock
  • throwing cancellation callbacks being diagnosed without replacing the signal exit policy
  • Ctrl+Break on Windows plus unclaimed SIGQUIT/ControlBreak on Unix
  • unsupported iOS-family platform selection, including Mac Catalyst
  • profile inheritance, unprofiled caller ownership, and explicit embedded opt-in
  • external provider/host diagnostics, one-shot caller-token identity, and interactive linked-token documentation
  • linked handler-token lifetime

Validation

  • dotnet build src/Repl.slnx -c Release -warnaserror --no-restore --nologo0 warnings, 0 errors
  • full suite — 1,499 total; 1,498 passed; 0 failed; 1 existing opt-in MCP Inspector smoke test skipped
  • ./eng/ci/process-signal-stress.sh50/50 unit iterations × 17 tests and 20/20 integration iterations × 8 tests
  • npx --yes markdownlint-cli2@0.19.1 CHANGELOG.md docs/best-practices.md docs/configuration-reference.md src/Repl.Defaults/README.md0 errors
  • git diff --check — clean

Follow-up design work

Kept out of this focused change and tracked separately:

@autocarl autocarl changed the title fix(runtime): cancel standalone runs on SIGTERM fix(runtime): cancel standalone runs on process signals Sep 3, 2026
@carldebilly

Copy link
Copy Markdown
Member

@codex A review of this please.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-10T18:44:08.252266Z c06aad9 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5d5ae4286b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Defaults/ReplApp.cs Outdated
Comment thread src/Repl.Core/Console/CancelKeyHandler.cs Outdated
Comment thread src/Repl.Defaults/ProcessSignalCancellationScope.cs Outdated
@autocarl

autocarl commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Review-hardening update, superseding the earlier summary, is published through 3a4b0ce.

Highlights:

  • keeps an unprofiled ReplApp.Create() caller-owned while CLI/default-interactive profiles opt into Automatic;
  • handles Ctrl+Break as Repl's 130 compatibility policy on Windows, while leaving Unix SIGQUIT/ControlBreak unclaimed;
  • keeps Mac Catalyst on the unsupported iOS-family path because .NET 10 compiles PosixSignalRegistration.PlatformNotSupported there;
  • diagnoses ignored Automatic requests on externally managed overloads: one-shot handlers receive the caller token unchanged, while interactive commands use a linked command-scoped token for Ctrl+C;
  • isolates and diagnoses consumer cancellation-callback failures without masking coordinator faults or replacing an established signal exit policy;
  • keeps production and test console-key filtering on one shared path;
  • adds executable eng/ci/process-signal-stress.sh so the concurrency evidence is replayable.

Exact-head local validation:

  • build: 0 warnings, 0 errors;
  • full suite: 1,498 passed, 0 failed, 1 existing opt-in skip;
  • stress: 50/50 × 17 unit tests and 20/20 × 8 process tests;
  • markdownlint: 0 errors;
  • git diff --check: clean.

Broader follow-ups remain tracked in #81, #82, #83, and #84. Exact-head CI for 3a4b0ce is green on Ubuntu, macOS, and Windows; pack, documentation lint, and real-shell smoke jobs also passed. The PR-only release job was skipped as expected. Run: https://github.com/yllibed/repl/actions/runs/33785494030

@carldebilly

Copy link
Copy Markdown
Member

@codex a review please.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 3a4b0ce02c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@carldebilly
carldebilly marked this pull request as ready for review September 4, 2026 03:13
@carldebilly

Copy link
Copy Markdown
Member

@codex last review?

@chatgpt-codex-connector

Copy link
Copy Markdown

Review Finding

[P2] Restored signal registrations use an invalidated generation

RegistrationFaultScope.Dispose() restores the previous console and SIGTERM registration objects and restores s_registrationsInitialized, but then increments s_generation. Those restored callbacks captured the prior generation when they were created, so they will subsequently return NotHandled. Because the initialized flag is also restored, later scopes will not reinstall working callbacks. Thus, after either registration-failure test runs, real process signals in the same test process are silently no longer bridged. Preserve and restore the original generation, or dispose the old registrations and install fresh ones after fault injection instead. src/Repl.Defaults/ProcessSignalCoordinator.csL292-L309

No code changes were made, so no commit or follow-up pull request was created.

Testing

  • git diff --check 3a4b0ce02c..HEAD
  • ⚠️ dotnet test src/Repl.Tests/Repl.Tests.csproj -c Release --no-restore --nologo --filter FullyQualifiedName~Given_ProcessSignalCancellationScope (dotnet is not installed in this environment)

View task →

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c57fa296df

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Defaults/ReplApp.cs Outdated
carldebilly added a commit that referenced this pull request Sep 10, 2026
Four mediums, two of which this branch introduced in the previous commit.

- The interactive/one-shot divergence on OperationCanceledException is now
  documented instead of excused. A one-shot run reserves Cancelled for the
  caller's own token; an interactive session treats every OCE escaping a command
  as an abort, Ctrl+C and a self-cancelling handler alike. The enum doc claimed
  the loop "cannot tell who asked to stop" — false, commandCts.IsCancellationRequested
  is in scope — and claimed Cancelled covers an abandoned prompt, which emits an
  aborted mark carrying no code. Behaviour deliberately unchanged; the three mark
  tests now say they exercise a self-raising handler rather than Ctrl+C, which no
  test actually simulates.
- The swallow contract on TryWriteCommandEndAsync named the exit-code table and
  ExitCodes.Resolver, neither of which it can reach: it takes an already-resolved
  int? and both call sites pass null. Restored to the mark-write wording, and the
  parallel rule at the resolve call site loses its third restatement.
- A hosted-lifecycle diagnostic printed only the coordinator's wrapper message,
  which names the failing service and not the reason. HostedServiceLifecycleException
  always carries an inner exception, so both diagnostics now name it as
  {Type}: {Message}, the shape TryWriteResolverDiagnostic already uses.
- The pre-cancellation check in RunAsync(args, IReplHost, IServiceProvider, ...)
  moved inside the session scope, still ahead of the overlay that resolves the
  caller's factories, so a resolver and its diagnostic reach the host's writers
  rather than Console.Error.

ExitCodes.Interrupted and the Interrupted kind are now documented as inert: no
public API produces that kind, so the entry does nothing until in-framework signal
handling (#80) lands. Four lenses raised it independently; the append-only enum
promise is why the member stays rather than being dropped.

Also: the unknown-output-format refusal moves to stderr, which is the rule this
branch introduced for framework diagnostics and the last site still writing into
the data channel; ClassifyResult builds its Success outcomes through the factory
set rather than the positional constructor; the TryStopHostedServicesAsync summary
no longer claims a suppressed *outcome* travels — only a suppressed exception does,
a returned outcome is replaced; ReplExecutionOutcome.Result stops promising the
carried refusal was rendered; the duplicate token-guard bullet and three
intra-branch "previously" claims are out of the CHANGELOG; best-practices notes
that HandlerError, HandlerException and FrameworkError all default to 1.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
autocarl and others added 11 commits September 10, 2026 07:04
Bridge SIGTERM into the standalone RunAsync cancellation token so handlers can complete cleanup before the process exits with code 143. Keep externally hosted runs caller-owned and expose an opt-out for custom process lifecycles.\n\nAdd a real child-process regression test for yllibed#79.
Handle SIGINT cooperatively for standalone runs on Unix and Windows while yielding to the existing interactive CancelKeyHandler semantics. Keep the embedded console profile caller-owned by default and cover second-signal fallback with a real child process.
Centralize Ctrl+C and SIGTERM ownership in process-wide coordinators. Preserve profile defaults, drain cancellation epochs before reset, and resolve exit codes only after releasing signal ownership. Add deterministic race coverage plus real sync/async signal integration tests.
Document profile inheritance, first- and second-signal behavior, 130/143 conventions, exit-code precedence, token lifetime, diagnostics, host ownership, and platform limitations.
Preserve caller ownership for unprofiled and externally managed runs, cover Ctrl+Break and Mac Catalyst, diagnose callback and registration failures, and add deterministic concurrency plus replayable stress coverage.
Keep Mac Catalyst on the unsupported mobile path, leave Unix SIGQUIT to the operating system, and clarify one-shot versus interactive cancellation-token contracts with real-process regression coverage.
… None

Make `ProcessSignalHandlingMode.None` the zero value so an unset configuration
field or a zero-initialized value agrees with the caller-owned application
default. Enum values are baked into consumer assemblies, so this is only free
before release; every existing use is by name.

Treat a rejected signal registration as a degraded bridge rather than a fatal
error: diagnose it, latch the attempt, and continue caller-owned, matching the
unsupported-platform path. Automatic is the CLI-profile default, so a
restricted environment previously turned a working command into one that never
executed, for that run and every later run in the process.

Also:

- run the process-signal stress harness in CI at low iteration counts, so the
  epoch races it guards are gated instead of relying on a local run
- state signal ownership, exit codes and the per-run opt-out in the
  `UseCliProfile` and `UseDefaultInteractive` summaries, where a consumer meets
  them, and pin the interactive profile's ownership with a test
- record the exit-code and handler-token changes for existing profile users
  under a CHANGELOG `Changed` heading
- assert the second-signal escalation diagnostic, list the unclaimed SIGQUIT
  exit code in the reference table, and cover each platform flag on its own row
  instead of restating the predicate once
- scope the race and reentrancy test descriptions to what they actually
  exercise: the gate serializes both dispatches, so neither proves atomicity
The script built the solution without `--no-restore`, so the incremental restore
audited no projects and tripped the CI-only NuGet audit assertion in
`src/Directory.Solution.targets` whenever the caller had already restored.

Follow the repository convention used by every workflow job: an explicit
`dotnet restore --force`, then a build with `--no-restore`. The script stays
self-contained when run on its own.
Three parts of this contract are hard to hold in the head from prose alone:
which of the three inputs decides a run's mode, how the process-wide epoch
moves between its states, and which console key each owner actually claims.

Add a mermaid diagram for each, next to the prose it illustrates. They carry
the skeleton only — the epoch diagram labels its edges with the step numbers of
the list above it rather than restating them, so the two cannot drift apart.

Each diagram was rendered and read before landing; the ownership flow also
records that a rejected registration degrades to caller-owned handling.
An independent review of the previous three commits found the registration
fault scope was a trap. Registrations capture the generation counter they were
created under; the scope put a saved registration object back after the counter
had moved, so it came back permanently stale. Any scope test running after a
fault test then got `NotHandled` instead of a claimed signal, and the latch made
sure the bridge was never reinstalled. It only stayed green because MSTest
happens to run this class in declaration order, and the stress harness loops
exactly this class.

Replace it with an isolation scope that tears registrations down on both entry
and exit and lets the next run install fresh ones. A regression test warms up
real registrations first, since the trap only exists when there is something to
restore.

Also from that review:

- collapse the two registration-degradation channels — an `out bool` for
  "unsupported" and a nullable record for "rejected" — into one outcome with a
  single diagnostic site, so a third reason does not need a third channel
- let the fault be injected after the SIGTERM registration, which is the only
  ordering that reaches the orphaned-registration cleanup, and cover it
- correct three claims that were wrong: `IsCancellationRequested` does not throw
  on a disposed source, it silently reports `false`, which is the dangerous case
  a consumer needs told; `UseDefaultInteractive` does not report 130 for a Ctrl+C
  inside an interactive command, which cancels only that command; and the
  console-key result enum is aggregated across handlers, so callers are not the
  only reader of its third state
- correct the two diagrams the previous commit got wrong: a run whose bridge
  failed to install still receives a linked run-scoped token, and a degraded
  process still starts and stops runs, it just never reaches Claimed
- state the ownership rule once on `ProcessSignalHandlingMode.Automatic` and have
  both profile summaries point at it, rather than three drifting copies
- assert that the run-scoped token is linked to the caller token, not merely
  distinct from it
- rename the concurrency test to what it asserts, now that its description
  admits it cannot prove atomicity
- give the stress harness its own CI job, so unrelated setup cannot silently
  drop signal coverage and the check name describes what it gates
yllibed#85 shipped ReplExecutionOutcomeKind.Interrupted and ExitCodes.Interrupted inert,
as the seam this PR needed. This wires them, so the two changes tell one story.

The signal scope used to overwrite an already-resolved exit code after the run:

    return runExitCode != 0 || signalExitCode is null ? runExitCode : signalExitCode.Value;

That left ExitCodes.Interrupted with nothing to govern, kept the kind unreachable,
hid signalled runs from ExitCodes.Resolver — documented as seeing every final
outcome — and handed a resolver two outcomes for one run, the defect yllibed#85 spent a
wave eliminating.

The run now reports an outcome and the signal reclassifies it, with exactly one
resolve at the outermost entry. ReplApp's internal paths return an ExecutionOutcome
rather than an int — RunOutcomeAsync for the dispatcher, RunHostedLifecycleOutcomeAsync
for the hosted lifecycle, both feeding the single ResolveProcessExitCode — which is
the shape yllibed#85 introduced for the hosted wrapper and which the signal scope needed
for the same reason. No public signature changes.

Precedence is now a stated rule rather than an int comparison:
ExecutionOutcome.IsInterruptible reclassifies a clean or cancelled run, while one
that already produced a refusal or a failure keeps reporting it — replacing a usage
error with 130 would hide why the command was wrong. That predicate is pure, so it
is asserted kind-by-kind instead of through a race no test could sequence.

ProcessSignalCancellationScope.ResolveExitCode is gone: it was dead in production
once the reclassification replaced it, and keeping it would have been the surface
with no producer that the review panel flagged on Interrupted itself.

Defaults are unchanged — 130 for SIGINT/Ctrl+Break, 143 for SIGTERM — and the three
new end-to-end guards were verified red before the wiring. 767 tests green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carldebilly added a commit to autocarl/repl that referenced this pull request Sep 10, 2026
Every run that reaches the core pipeline now ends in a ReplExecutionOutcome
(ReplExecutionOutcomeKind: Success, Help, UsageError, BindingError,
HandlerError, HandlerExitCode, HandlerException, Cancelled, Interrupted,
FrameworkError). The kind is mapped to an integer once, at the top of
CoreReplApp.ExecuteCoreAsync, through the new ReplOptions.ExitCodes table
(ExitCodeOptions) and then handed to the optional ExitCodes.Resolver hook.

Default codes change: framework refusals (unknown command, ambiguous prefix,
invalid or colliding option, context validation, unknown output format,
ambient misuse) and binding failures exit 2 instead of 1; handler failures
stay 1, help and success stay 0; Results.Exit keeps its code verbatim.
ExitCodes.Cancelled (int?, unset by default) turns a caller-token
cancellation into an exit code instead of letting the exception escape.

ReplExecutionContext.Result exposes the handler return value to middleware,
readable and replaceable after next(); ReplNext and Use are unchanged.

The interactive loop resolves shell-integration mark codes through the same
table and resolver, including Ctrl+C (conventional 130 unless remapped).
MCP sub-invocations keep the built-in defaults and skip the resolver.
Repl.Testing still raises TimeoutException when the app under test maps
Cancelled. Interrupted is reserved for the process-signal bridge (yllibed#79/yllibed#80).

Closes yllibed#81.
@carldebilly
carldebilly force-pushed the agent/issue-79-signal-handling branch from 09a64b9 to c68ad81 Compare September 10, 2026 11:23
Closes the last open review thread on this PR, which reported that a signal
arriving while IHostedService.StartAsync was pending returned 1 instead of
130/143: the coordinator wraps the cancellation in a HostedServiceLifecycleException,
that was classified as a lifecycle failure, and the resulting non-zero code then
defeated the signal's own.

Both paths now report the interruption, and the test covers both rather than the
one I could reason about. With a cancellation policy configured the startup failure
classifies as Cancelled, which the signal reclassifies as Interrupted; with none it
propagates the inner OperationCanceledException, which the signal path catches and
reclassifies the same way. Either way the run exits 130, and ExitCodes.Cancelled
does not win over the signal — the run was interrupted, not merely cancelled.

Verified red before the wiring in the previous commit, alongside the other four
guards. 769 tests green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 92c95b7c6d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
Comment thread src/Repl.Defaults/ReplApp.cs
Comment thread src/Repl.Defaults/ReplApp.cs
…ontain diagnostics

Three findings from the review round on 92c95b7. The first is mine, from the
previous commit.

- An explicit exit code was lost when its payload rendering was cancelled.
  ExecuteMatchedCommandAsync classified the result *after* rendering it, so a
  signal arriving mid-render escaped before ClassifyResult ran: the outcome became
  Cancelled, the signal reclassified it as Interrupted, and the run reported
  130/143 instead of the handler's own non-zero code — against the precedence both
  IExitResult and this PR document. The result is now classified first, and a
  cancellation during rendering keeps a HandlerExitCode outcome. A zero code still
  yields to the interruption, since the handler used it to report nothing, and a
  test pins that half so the fix cannot over-reach.

- An undefined ProcessSignalHandlingMode fell through a negative test into
  automatic mode. Numeric configuration or deserialization can produce one, and
  the consequence was silently acquiring process-wide signal ownership and swapping
  the handler's token for a run-scoped one. It is now rejected. The profile default
  is framework-set and always defined, so a bad value can only come from the
  caller's ReplRunOptions, which is what the exception names.

- ProcessSignalCoordinator.WriteDiagnostic caught only IOException and
  ObjectDisposedException, so an application-supplied TextWriter throwing anything
  else escaped from inside a signal callback — before it returned its suppression
  decision, which replaces cooperative cleanup with immediate process termination.
  It now contains any failure, matching the four best-effort diagnostic writers the
  exit-code policy established, each with the same CA1031 justification.

Three of the four new tests were verified red before their fix; the fourth is the
over-reach guard described above. 773 tests green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dd6bbde9b6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/Repl.Core/CoreReplApp.Execution.cs Outdated
Comment thread src/Repl.Defaults/ProcessSignalHandlingMode.cs
Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
…tuple path

Three findings from the review round on dd6bbde. The first two are mine, from the
commit that introduced the preservation.

- The guard keyed only on the outcome kind, so a transformer raising
  OperationCanceledException on its own account while rendering an exit result's
  payload returned the handler's code — a broken renderer passing for an
  intentional exit, and inconsistent with the same transformer failing on a plain
  result, which is a HandlerException. PreservesExplicitExitCode now also requires
  IsCallerCancellation, so only a cancellation the run owns keeps the code.

- RenderTupleResultAsync had the same loss the scalar path did: it rendered each
  element before classifying the last, so a tuple ending in Results.Exit(7, payload)
  lost the code when that element's rendering was cancelled. The per-element render
  moved into RenderTupleElementAsync, which shares the preservation rule and keeps
  the loop under the method-length cap.

  Worth recording what the first version of that test got wrong: it signalled while
  rendering the *first* element, and 130 was the correct answer there — the exit
  result had not been classified yet, so there was no code to preserve. The test now
  signals on the exit payload itself, which is the case that was reported.

- The signal-bridge platform predicate named iOS but not Mac Catalyst, which this PR
  documents as unsupported. I could not establish from the .NET documentation whether
  OperatingSystem.IsIOS() reports true there — what the docs do state is that the
  compatibility analyzer treats MacCatalyst as a superset of iOS for attribute
  inference, which is a different question. The predicate now names Mac Catalyst
  explicitly, so it states what it means either way.

Both behavioural guards verified red before their fix. 776 tests green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ssue

The comment said the extraction was "tracked as a follow-up" without naming one,
which was an invented reference until yllibed#89 existed. It now cites the issue.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@carldebilly

Copy link
Copy Markdown
Member

@codex a review please

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93b1eb7212

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread CHANGELOG.md
Comment thread src/Repl.Core/CoreReplApp.Execution.cs
Comment thread src/Repl.Defaults/ProcessSignalCoordinator.cs Outdated
Three findings from the review round on 93b1eb7. Two are mine, from the commit
that narrowed the preservation two rounds ago.

- The predicate preserved *every* HandlerExitCode against a caller cancellation,
  including a zero one. So with signal handling off, a handler returning
  Results.Exit(0, payload) whose transformer observed the caller's token exited 0 —
  a cancelled run reporting success, swallowing the cancellation instead of
  propagating it or applying ExitCodes.Cancelled. Automatic signal mode masked it,
  because ApplySignalInterruption reclassifies a zero exit afterwards; nothing
  corrects it on the other overloads. It now also requires !IsSuccessLike, which is
  the same line a process signal already used to decide whether to reclassify a
  run, so the two sites share one notion of "had something to report".

- CHANGELOG.md contradicted itself four lines apart: a compatibility note still
  said Interrupted and ExitCodes.Interrupted were inert and unreachable, while the
  section below documents automatic mode producing exactly that kind. I updated the
  XML docs and docs/ when wiring it and missed this one.

- Windows routes Ctrl+C and Ctrl+Break through one console callback, which
  hard-coded the name SIGINT, so Ctrl+Break produced "Received SIGINT" against a
  mode that documents the two keys separately. The console handler delegates now
  carry the ConsoleSpecialKey: the interactive handler ignores it, since both keys
  cancel a command identically, and the standalone one names the event. SIGINT is
  also simply the wrong word on Windows.

Both behavioural guards verified red before their fix, and a third test pins that
narrowing the predicate did not undo the preservation it exists for. 779 tests green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@carldebilly
carldebilly merged commit 28dc7d0 into yllibed:main Sep 10, 2026
8 checks passed
carldebilly added a commit that referenced this pull request Sep 10, 2026
The file arrived as a side effect of a feature PR (fde24c4, the hidden-option
work) rather than as a release-process decision, and nothing consumes it: no
workflow, no script, no packaging step, and no document links to it.

Meanwhile CI already publishes what it was trying to be. The release job runs
`gh release create "v${VERSION}" --generate-notes`, so every published version
gets notes generated from the merged pull requests and anchored to the version a
consumer installs. The file could never be anchored that way — its own header said
so, because Nerdbank.GitVersioning assigns the version at pack time — and a review
lens correctly flagged that a breaking change pinned to "the commit closing issue

It was also the single point of conflict between concurrent pull requests, which
is how the question came up: it was the only conflicting file when #85 merged and
again when #80 rebased, with three more PRs open behind them.

The one piece of guidance that lived only here is migrated: the recipe for
restoring the pre-policy exit codes, with its test-suite and MCP consequences, now
sits in docs/configuration-reference.md beside the table it talks about. The rest
was already covered by the topic pages — docs/commands.md:172 for the additive
`isHidden` export fields, docs/testing-toolkit.md for CommandExecution.ExitCode.

docs/publishing.md now states where release notes come from and what that asks of
a PR description, so the next person does not recreate the file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carldebilly added a commit that referenced this pull request Sep 10, 2026
Rebased onto the merged #80, which had added three CHANGELOG sections after this
branch was written. Taking the deletion is right — nothing reads the file and the
release job generates versioned notes — but deleting it unaudited would have lost
what only it recorded, which is the mistake the review caught on this PR the first
time.

#80 documented its rule well: docs/configuration-reference.md already covers the
run-scoped token, the mode table, the epoch machine and the platform matrix. What
lived only in the changelog was the *migration consequence* for an existing app,
so that moves next to the rule it belongs to:

- taking signal ownership by default under UseCliProfile/UseDefaultInteractive, and
  that ProcessSignalHandlingMode.None restores the previous behaviour;
- interruption now resolving to 130/143 where OS-default termination used to apply,
  and what that does to a script treating any non-zero code as failure;
- the handler-token change and how it fails — ObjectDisposedException from Register
  or WaitHandle, and IsCancellationRequested silently continuing to report false,
  which the existing "must not retain it" sentence did not say.

That last one is the reason this audit was worth doing rather than assumed: a rule
tells a reader what to do, and only the consequence tells them what breaks if they
did not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
carldebilly added a commit that referenced this pull request Sep 10, 2026
The file arrived as a side effect of a feature PR (fde24c4, the hidden-option
work) rather than as a release-process decision, and nothing consumes it: no
workflow, no script, no packaging step, and no document links to it.

Meanwhile CI already publishes what it was trying to be. The release job runs
`gh release create "v${VERSION}" --generate-notes`, so every published version
gets notes generated from the merged pull requests and anchored to the version a
consumer installs. The file could never be anchored that way — its own header said
so, because Nerdbank.GitVersioning assigns the version at pack time — and a review
lens correctly flagged that a breaking change pinned to "the commit closing issue

It was also the single point of conflict between concurrent pull requests, which
is how the question came up: it was the only conflicting file when #85 merged and
again when #80 rebased, with three more PRs open behind them.

The one piece of guidance that lived only here is migrated: the recipe for
restoring the pre-policy exit codes, with its test-suite and MCP consequences, now
sits in docs/configuration-reference.md beside the table it talks about. The rest
was already covered by the topic pages — docs/commands.md:172 for the additive
`isHidden` export fields, docs/testing-toolkit.md for CommandExecution.ExitCode.

docs/publishing.md now states where release notes come from and what that asks of
a PR description, so the next person does not recreate the file.
carldebilly added a commit that referenced this pull request Sep 10, 2026
Rebased onto the merged #80, which had added three CHANGELOG sections after this
branch was written. Taking the deletion is right — nothing reads the file and the
release job generates versioned notes — but deleting it unaudited would have lost
what only it recorded, which is the mistake the review caught on this PR the first
time.

#80 documented its rule well: docs/configuration-reference.md already covers the
run-scoped token, the mode table, the epoch machine and the platform matrix. What
lived only in the changelog was the *migration consequence* for an existing app,
so that moves next to the rule it belongs to:

- taking signal ownership by default under UseCliProfile/UseDefaultInteractive, and
  that ProcessSignalHandlingMode.None restores the previous behaviour;
- interruption now resolving to 130/143 where OS-default termination used to apply,
  and what that does to a script treating any non-zero code as failure;
- the handler-token change and how it fails — ObjectDisposedException from Register
  or WaitHandle, and IsCancellationRequested silently continuing to report false,
  which the existing "must not retain it" sentence did not say.

That last one is the reason this audit was worth doing rather than assumed: a rule
tells a reader what to do, and only the consequence tells them what breaks if they
did not.
carldebilly added a commit that referenced this pull request Sep 10, 2026
The servicing flow said "merge or cherry-pick the fix and commit it", and the release-notes section
said `--generate-notes` builds the body from merged pull requests. Both are true, and together they
describe a patch release that says nothing: a cherry-pick reaches the branch outside a pull request,
so it contributes no entry, and this branch is the one that deleted CHANGELOG.md.

Measured rather than assumed. `POST /releases/generate-notes` for the twelve commits between
v0.12.0-dev.45 and this branch head — real commits, no merged pull request, since #88 is still open —
returns a body of one line, the compare link, with no `What's Changed` heading at all. The same call
across a range containing #80 returns its title, author and link.

So the servicing section now asks for a pull request targeting the release branch and says why, with
editing the release body by hand as the fallback when a fix lands as a direct commit; the workflow
passes `--generate-notes` unconditionally and cannot supply notes for that path. The release-notes
section states the empty case alongside what it already said about PR descriptions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

One-shot RunAsync(args, ct) never cancels: no signal source outside the interactive session

2 participants